Parse symmetry info and show in cell tab #328

unkcpz · 2022-07-12T08:29:34Z

~~Move the cell tab to front~~ Can be set when create widget.
parse symmetry info using spglib
~~Using ase.Atom instead of ase.Cell in StructureDataViewer as active traitslet talk to other widget.~~
Missing unit symbol 'Angstrom'
~~drop ase.Cell for structure viewer, use only ase.Atoms as trait.~~
When atom selected, automatically change to 'selection' tab.

unkcpz · 2022-07-31T14:33:20Z

yakutovicha · 2022-08-02T14:08:56Z

@unkcpz I see there are still some non-completed tasks for this PR. Whenever this is ready please assign relevant people for the review.

unkcpz · 2022-08-08T16:37:37Z

There is still one thing probably good to have. When the structure is not chosen or uploaded the StructureDataViewer is hidden. If you all agree with this, since I am on this part of code, I can quickly add this feature to this PR.

unkcpz · 2022-08-08T16:38:52Z

The result of the widget after these change would be:

And once the atom selected, the tab will automatically change to selection

csadorf · 2022-08-09T06:25:07Z

@unkcpz Please re-request my review when this is ready, thank you!

unkcpz · 2022-08-09T06:29:10Z

@csadorf Yes, it is ready to review.

aiidalab_widgets_base/utils/__init__.py

csadorf · 2022-08-09T08:19:28Z

aiidalab_widgets_base/utils/__init__.py

@@ -153,3 +154,12 @@ class StatusHTML(_StatusWidgetMixin, ipw.HTML):
    @traitlets.observe("message")
    def _observe_message(self, change):
        self.show_temporary_message(change["new"])
+
+
+def ase2spglib(ase_structure: Atoms):


Can you declare the return type here?

aiidalab_widgets_base/utils/__init__.py

aiidalab_widgets_base/viewers.py

csadorf · 2022-08-09T08:25:09Z

aiidalab_widgets_base/viewers.py

@@ -167,15 +168,16 @@ class _StructureDataBaseViewer(ipw.VBox):
    selection = List(Int)
    selection_adv = Unicode()
    supercell = List(Int)
-    cell = Instance(Cell, allow_none=True)
+    cell = Instance(Cell, allow_none=True)  # Will be deprecated


Do we need to add a deprecation warning then?

What is the motivation for this?

per 1: Maybe (only if other app use this widget and directly use cell traitslet, @yakutovicha if the apps from empa using this directly).

per 2: The motivation to replace cell with ase_structure is to read the symmetry information from the traitlets . From my point of view, it makes more sense to store and use full structure.

I'll wait for @yakutovicha to comment on this.

The structure trait of the StructureDataViewer child class serves the purpose of ase_structure. You can extract all the information from it.

per 2: The motivation to replace cell with ase_structure is to read the symmetry information from the traitlets . From my point of view, it makes more sense to store and use full structure.

We actually thought of _StructureDataBaseViewer as a base class that could be used also for the trajectory viewers. I would, therefore, be very careful in changing this part.

I would at least do this particular change in another PR so that we could carefully consider all the pros and cons.

Just to mention that @danielhollas is currently preparing things for the trajectory viewer, so we might want to revisit the design of _StructureDataBaseViewer class.

I can create a separate issue to collect the ideas. What do you think @unkcpz and @danielhollas?

Thanks @yakutovicha please open an issue for trajectory.
I will revert this part in the pr. Basically I need the ase.Atom only for parse the symmetry info.

Look at the implementation of this PR again, if I revert back, there is no essential changes left.
I think if we call this class a 'StructureData' viewer, the positions info should be included with is not in the original implementation, which makes not much sense to me.
@danielhollas Could you have a look at this implementation to see if this will conflict with your trajectory feature, if not, it is much easier to keep the base traitslet type change here.
Therefore, I'll remove the cell traitslet if it is fine for you.

@unkcpz yeah, I am just looking into this, the split between _StructureDataBaseViewer and StructureDataViewer is a bit unclear to me. I think for your use case here it would make most sense if you moved the displayed_structure traitlet here. Then you would have access to the ASE structure, you don't need extra ase_structure traitlet.

The structure trait of the StructureDataViewer child class serves the purpose of ase_structure. You can extract all the information from it.

Just a correction to this, the structure trait can be either Atoms or AiiDA node, whereas displayed_structure is guaranteed to be the ASE Atoms object. So that's why I think it would make sense to move here.

Regardless, I am fine with removing the cell traitlet. Since this is inside the private class, other things should not be using it anyway no?

I think I am a bit confused with the design here. Can we have a short meeting to discuss it?
The catch here is the _StructureDataBaseViewer has no positions info but I need to put the symmetry info in the cell tab which needs to use cell + position info. We need first to clarify the variable names used for our class since 'cell' sometimes only refers to lattice but in some libraries refers to the whole structure.

(This was for last time, please igrone me 💔)

Look at the implementation of this PR again, if I revert back, there is no essential changes left.

Not really, I guess the purpose of this PR is to have the symmetry information displayed. This can stay.

the positions info should be included with is not in the original implementation

sorry, I don't understand this, could you maybe rephrase?

Just a correction to this, the structure trait can be either Atoms or AiiDA node, whereas displayed_structure is guaranteed to be the ASE Atoms object. So that's why I think it would make sense to move here.

No, it accepts both Atoms and AiiDA node, but then they get immediately converted into an ASE object:

aiidalab-widgets-base/aiidalab_widgets_base/viewers.py

Lines 737 to 738 in cabc5df

@validate("structure")

def _valid_structure(self, change): # pylint: disable=no-self-use

Regardless, I am fine with removing the cell traitlet. Since this is inside the private class, other things should not be using it anyway no?

It will also stay as a public member of the derived classes, we still need to be careful about removing it.

[P.S.]
Overall, I would like to stress that generally, I agree - we might change the classes to better satisfy our needs. We might do it in a separate PR (as I was suggesting) or just bite the bullet and do everything here.

I try to be practical, that is why I suggest merging things that are clear and addressing the rest in a separate PR. If you want to do everything here - that is also fine with me. But keep in mind that it will cause some delays and long difficult-to-follow discussions.

aiidalab_widgets_base/viewers.py

danielhollas · 2022-08-17T16:07:52Z

aiidalab_widgets_base/viewers.py

-        if self.cell:
+    @observe("ase_structure")
+    def _observe_ase_structure(self, _=None):
+        if self.ase_structure:


@unkcpz In this logic, please take into account non-periodic systems by checking the pbc attribute of the ase object.

Good point! @danielhollas

I add a check and only parse and attach symmetry when pbc shows it is 3-D

@unkcpz it might also be nice to automatically hide the Cell tab in that case. Not sure how much work that would be so perhaps in another PR?

Probably can update the tab widget but then need to bring it back when 3D structure is selected again, seems a bit tedious to make it work correctly, I'll take a look.
Or we can make the widget support the 2D and 1D with big cell set like we have for [optimade-client]?(https://optimadeclient.materialscloud.io/). From where if you search structure in MC2D database. Not sure I remember how we do that, I'll check it.

Suggested change

if self.ase_structure:

if self.ase_structure and any(self.ase_structure.pbc):

danielhollas

@unkcpz I left a couple comments, let me know if my suggestions make sense.

danielhollas · 2022-08-17T22:55:43Z

aiidalab_widgets_base/viewers.py

@@ -167,15 +168,16 @@ class _StructureDataBaseViewer(ipw.VBox):
    selection = List(Int)
    selection_adv = Unicode()
    supercell = List(Int)
-    cell = Instance(Cell, allow_none=True)
+    cell = Instance(Cell, allow_none=True)  # Will be deprecated


@unkcpz yeah, I am just looking into this, the split between _StructureDataBaseViewer and StructureDataViewer is a bit unclear to me. I think for your use case here it would make most sense if you moved the displayed_structure traitlet here. Then you would have access to the ASE structure, you don't need extra ase_structure traitlet.

The structure trait of the StructureDataViewer child class serves the purpose of ase_structure. You can extract all the information from it.

Just a correction to this, the structure trait can be either Atoms or AiiDA node, whereas displayed_structure is guaranteed to be the ASE Atoms object. So that's why I think it would make sense to move here.

Regardless, I am fine with removing the cell traitlet. Since this is inside the private class, other things should not be using it anyway no?

danielhollas · 2022-08-17T23:18:40Z

aiidalab_widgets_base/viewers.py

-        if self.cell:
+    @observe("ase_structure")
+    def _observe_ase_structure(self, _=None):
+        if self.ase_structure:


@unkcpz it might also be nice to automatically hide the Cell tab in that case. Not sure how much work that would be so perhaps in another PR?

danielhollas · 2022-08-17T23:21:13Z

aiidalab_widgets_base/viewers.py

@@ -759,10 +796,10 @@ def _observe_structure(self, change):
        # Remove the current structure(s) from the viewer.
        if change["new"] is not None:
            self.set_trait("displayed_structure", change["new"].repeat(self.supercell))
-            self.set_trait("cell", change["new"].cell)
+            self.set_trait("ase_structure", change["new"])


I don't think this works in general case because the structure trait can be AiiDA node (StructureData or CifData). You would need to move this to the displayed_structure observer, but as mentioned in the other comment, I think it is easier to just move the displayed_structure trait to the parent class.

unkcpz · 2022-08-18T08:33:58Z

Thanks for the review! @danielhollas
I think I am a bit confused with the design here. Can we have a short meeting to discuss it (we can meet next aiidalab meeting or quickly set a meeting if you are available)?
The catch here is the _StructureDataBaseViewer has no positions info but I need to put the symmetry info in the cell tab which needs to use cell + position info. We need first to clarify the variable names used for our class since 'cell' sometimes only refers to lattice but in some libraries refers to the whole structure.

yakutovicha · 2022-08-18T10:09:21Z

Thanks for the review! @danielhollas I think I am a bit confused with the design here. Can we have a short meeting to discuss it (we can meet next aiidalab meeting or quickly set a meeting if you are available)? The catch here is the _StructureDataBaseViewer has no positions info but I need to put the symmetry info in the cell tab which needs to use cell + position info. We need first to clarify the variable names used for our class since 'cell' sometimes only refers to lattice but in some libraries refers to the whole structure.

I agree, we need to meet to clear things out.

Just as a brief mention: the idea to keep the structure trait outside of _StructureDataBaseViewer is coming from the generality principle. We wanted to reuse the same base class for the trajectory, where the trait would probably be called trajectory.

danielhollas · 2022-08-18T10:33:03Z

Sure, let's have a quick meeting. The next AiiDAlab meeting is after the Psi-k conference and I cannot attend unfortunately since I am at another conference in Rennes. Could we do a short meeting today or tomorrow afternoon?

@yakutovicha to clarify my suggestion, I would move the displayed_structure to the base class. That way there is a clearer separation of concern, since the base class is then in charge of what is displayed. The structure (or trajectory) traits would then stay in the concrete viewers. StructureDataViewer would then simply pass the structure info from structure trait to displayed_structure trait (converting to ase object from AiiDA node object), while the TrajectoryDataViewer would be passing the individual structure based on some selection widget.

yakutovicha · 2022-08-18T12:35:11Z

Could we do a short meeting today or tomorrow afternoon?

I am available both today and tomorrow. @unkcpz what about you?

unkcpz · 2022-08-18T12:38:36Z

I have time today 3pm-3:30pm and tomorrow after 4pm. If you can find time, let me know. Thanks!

danielhollas · 2022-08-18T13:06:02Z

@unkcpz @yakutovicha I am available now but tomorrow works as well.

yakutovicha · 2022-08-18T13:11:25Z

@unkcpz @yakutovicha I am available now but tomorrow works as well.

@danielhollas I've just sent you the link via email.

unkcpz · 2022-09-02T14:25:29Z

Hi @yakutovicha @danielhollas, I adapt the changes without touching the cell trait. I hope this is what we agreed on.

danielhollas

@unkcpz thanks, the code looks good to me. I have some suggestions / questions, but those could be done in subsequent PR if needed.

I did not actually test this, I am leaving that to somebody with more solid state background.

danielhollas · 2022-09-02T15:56:43Z

aiidalab_widgets_base/viewers.py

@@ -335,7 +340,8 @@ def change_camera(change):

    @observe("cell")
    def _observe_cell(self, _=None):
-        if self.cell:
+        # only update cell info when it is a 3D structure.
+        if self.cell and all(self.structure.pbc):


It would be nice to either completely hide the Cell tab when the structure is not periodic, or at least explicitly indicate that inside the tab.

Thanks for mentioning this. Actually, I made some effort to implement that but failed. The reason is that the code either hides the tab or adds a message inside the cell tab makes more sense to put in _cell_tab method, but in there the self.structure trait is not being set yet.
Adding that too this PR will bring more coupling. But you are right, if it is okay with you, I'll open an issue for this and leave it to the future after we have decoupled the base structure data view.

@unkcpz did you open the issue? If yes, please mention it here for the record

I just open the issue #351 for this.

danielhollas · 2022-09-02T15:57:03Z

aiidalab_widgets_base/viewers.py

@@ -335,7 +340,8 @@ def change_camera(change):

    @observe("cell")
    def _observe_cell(self, _=None):
-        if self.cell:
+        # only update cell info when it is a 3D structure.


I wonder why 2D structures are not supported?

There are two reasons that make this not easy to show. First, if it is a 2D structure, the spglib does not work well with fining the symmetry for 2D structure which has an different set of space group. Secondly, I have no idea what to show for the cell information, do we still show a, b, c with c set to a large value as mimicking the vacuum layer, or don't show the c and beta in the tab.

unkcpz

Thanks for review @danielhollas .

I don't know why I can't directly reply to the comment (they are all in Pending state), but has to reply with the review. I think it might be I have reply using the review code last time (The first comment is from last time, please ignore it).
Sorry for the mess.

unkcpz · 2022-08-18T08:32:12Z

aiidalab_widgets_base/viewers.py

@@ -167,15 +168,16 @@ class _StructureDataBaseViewer(ipw.VBox):
    selection = List(Int)
    selection_adv = Unicode()
    supercell = List(Int)
-    cell = Instance(Cell, allow_none=True)
+    cell = Instance(Cell, allow_none=True)  # Will be deprecated


I think I am a bit confused with the design here. Can we have a short meeting to discuss it?
The catch here is the _StructureDataBaseViewer has no positions info but I need to put the symmetry info in the cell tab which needs to use cell + position info. We need first to clarify the variable names used for our class since 'cell' sometimes only refers to lattice but in some libraries refers to the whole structure.

(This was for last time, please igrone me 💔)

unkcpz · 2022-09-02T16:15:12Z

aiidalab_widgets_base/viewers.py

@@ -335,7 +340,8 @@ def change_camera(change):

    @observe("cell")
    def _observe_cell(self, _=None):
-        if self.cell:
+        # only update cell info when it is a 3D structure.
+        if self.cell and all(self.structure.pbc):


Thanks for mentioning this. Actually, I made some effort to implement that but failed. The reason is that the code either hides the tab or adds a message inside the cell tab makes more sense to put in _cell_tab method, but in there the self.structure trait is not being set yet.
Adding that too this PR will bring more coupling. But you are right, if it is okay with you, I'll open an issue for this and leave it to the future after we have decoupled the base structure data view.

unkcpz · 2022-09-02T16:20:37Z

aiidalab_widgets_base/viewers.py

@@ -335,7 +340,8 @@ def change_camera(change):

    @observe("cell")
    def _observe_cell(self, _=None):
-        if self.cell:
+        # only update cell info when it is a 3D structure.


There are two reasons that make this not easy to show. First, if it is a 2D structure, the spglib does not work well with fining the symmetry for 2D structure which has an different set of space group. Secondly, I have no idea what to show for the cell information, do we still show a, b, c with c set to a large value as mimicking the vacuum layer, or don't show the c and beta in the tab.

yakutovicha

All good also from my side, thanks @unkcpz!

for more information, see https://pre-commit.ci

unkcpz · 2022-09-06T15:03:06Z

Okay, a bit late than the PR of @danielhollas (3 min 🥺), introduce a conflict here. I resolve the conflict and rebase it and should be fine now. Nothing is charged so I'll merge it without your approval.

unkcpz marked this pull request as draft July 12, 2022 08:29

unkcpz mentioned this pull request Jul 12, 2022

Polish the Structure Selection widget aiidalab/aiidalab-qe#244

Closed

4 tasks

unkcpz marked this pull request as ready for review August 8, 2022 16:35

unkcpz requested review from yakutovicha and csadorf August 8, 2022 16:35

csadorf removed their request for review August 9, 2022 06:24

unkcpz requested a review from csadorf August 9, 2022 06:28

unkcpz force-pushed the feature/qeapp/xx/structure-viewer-cell-first branch from 8f80b2e to 05d0f38 Compare August 9, 2022 06:35

csadorf reviewed Aug 9, 2022

View reviewed changes

csadorf suggested changes Aug 9, 2022

View reviewed changes

unkcpz changed the title ~~structure viewer cell first and parse symmetry info~~ structure ~~viewer cell first~~ and parse symmetry info Aug 9, 2022

unkcpz changed the title ~~structure ~~viewer cell first~~ and parse symmetry info~~ structure viewer cell first and parse symmetry info Aug 9, 2022

unkcpz requested a review from csadorf August 9, 2022 10:39

unkcpz mentioned this pull request Aug 9, 2022

Move cell tab to front in structure viewer aiidalab/aiidalab-qe#265

Merged

danielhollas reviewed Aug 17, 2022

View reviewed changes

unkcpz force-pushed the feature/qeapp/xx/structure-viewer-cell-first branch from f907b3a to 8fc6bb2 Compare August 17, 2022 22:54

unkcpz requested a review from danielhollas August 17, 2022 22:56

danielhollas requested changes Aug 17, 2022

View reviewed changes

unkcpz force-pushed the feature/qeapp/xx/structure-viewer-cell-first branch 2 times, most recently from c05e536 to 958b10a Compare September 2, 2022 14:23

unkcpz requested a review from danielhollas September 2, 2022 14:25

danielhollas previously approved these changes Sep 2, 2022

View reviewed changes

unkcpz commented Sep 2, 2022

View reviewed changes

yakutovicha previously approved these changes Sep 6, 2022

View reviewed changes

unkcpz changed the title ~~structure viewer cell first and parse symmetry info~~ Parse symmetry info and show in cell tab Sep 6, 2022

unkcpz and others added 11 commits September 6, 2022 14:55

swap tab order of structure view

70287ce

add symmetry info parser

8d79acb

Using ase_structure as traitslet for StructureViewer

6518ada

Unit Angstrom in cell viewer

ab8fda1

Move to selection tab when atom selected

302b001

? spglib to dependency

221a996

move function for easy review

ac87e89

review

134c7d2

check pbc

b815ae1

without adding ase_structure trait

bef278e

[pre-commit.ci] auto fixes from pre-commit.com hooks

3199fc2

for more information, see https://pre-commit.ci

unkcpz dismissed stale reviews from yakutovicha and danielhollas via 3199fc2 September 6, 2022 14:56

unkcpz force-pushed the feature/qeapp/xx/structure-viewer-cell-first branch from 02a864f to 3199fc2 Compare September 6, 2022 14:56

r

821fcdc

unkcpz force-pushed the feature/qeapp/xx/structure-viewer-cell-first branch from 6d13de7 to 821fcdc Compare September 6, 2022 15:00

[pre-commit.ci] auto fixes from pre-commit.com hooks

5e6d3b6

for more information, see https://pre-commit.ci

unkcpz merged commit 770c641 into master Sep 6, 2022

unkcpz deleted the feature/qeapp/xx/structure-viewer-cell-first branch September 6, 2022 15:05

unkcpz mentioned this pull request Sep 20, 2022

the "Cell" tab is missing units (Angstrom). aiidalab/aiidalab-qe#290

Closed

	@validate("structure")
	def _valid_structure(self, change): # pylint: disable=no-self-use

	if self.ase_structure:
	if self.ase_structure and any(self.ase_structure.pbc):

Parse symmetry info and show in cell tab #328

Parse symmetry info and show in cell tab #328

Conversation

unkcpz commented Jul 12, 2022 • edited Loading

unkcpz commented Jul 31, 2022

yakutovicha commented Aug 2, 2022

unkcpz commented Aug 8, 2022

unkcpz commented Aug 8, 2022 • edited Loading

csadorf commented Aug 9, 2022

unkcpz commented Aug 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unkcpz Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhollas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unkcpz commented Aug 18, 2022

yakutovicha commented Aug 18, 2022

danielhollas commented Aug 18, 2022

yakutovicha commented Aug 18, 2022

unkcpz commented Aug 18, 2022

danielhollas commented Aug 18, 2022

yakutovicha commented Aug 18, 2022

unkcpz commented Sep 2, 2022

danielhollas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhollas Sep 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unkcpz left a comment • edited Loading

Choose a reason for hiding this comment

unkcpz Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yakutovicha left a comment

Choose a reason for hiding this comment

unkcpz commented Sep 6, 2022

unkcpz commented Jul 12, 2022 •

edited

Loading

unkcpz commented Aug 8, 2022 •

edited

Loading

unkcpz Aug 18, 2022 •

edited

Loading

danielhollas Sep 2, 2022 •

edited

Loading

unkcpz left a comment •

edited

Loading

unkcpz Aug 18, 2022 •

edited

Loading